fix: review-thread wave 4 — invocation forwarding, codex path regex, tool_input scope, CLI help, README targets, worker-root cleanup, probe redaction/teardown, concurrent doctor scan - #397

Keep the cleanup cap alive through shutdown

When a transport teardown stalls and Workbench shuts down before this 10-second cap fires, cap.unref() permits the process to exit without removing the probe's plugin-data directory. The production lifecycle only closes McpProbeRoutes and never awaits mcpProbe.settle(), so the newly detached cleanup has no referenced handle guaranteeing completion; keep this cap referenced or join settle() during server shutdown.

Useful? React with 👍 / 👎.

Fixed in this PR (5ad8613). Two changes: (1) the 10 s teardown cap in #removePluginDataAfter is now kept referenced (no unref()), so a stalled teardown can no longer let the process exit before the plugin-data removal runs — the timer is still cleared the moment the teardown settles; (2) the dev server's closeForeground now awaits mcpProbe.settle() (in a finally after foreground.close()), so Workbench shutdown joins detached probe cleanups, bounded by that cap. Proof: new integration test mcp-probe-dev-server.test.ts :: joins detached probe plugin-data cleanup into Workbench shutdown starts a real dev server with a stdio transport whose close() takes 400 ms, probes it (response returns while the transport is still closing and plugin data still exists), then asserts server.close() resolves only after the transport close settled and the plugin-data directory is gone. Red without the change, green with it.

Acknowledged — with cap.unref() and nothing awaiting mcpProbe.settle() on shutdown, a stalled transport teardown can leave the probe's plugin-data directory behind. This PR is still open; owner is fix/review-threads-wave4. Tracked by the late-review sweep; not fixed on main.

Wait for active probes before settling teardown

When server.close() is called while a probe is still connecting or listing tools, ForegroundServer only fences new MCP probe requests; it does not await the active handler. At this point McpProbeService.settle() can observe an empty #pendingTeardowns set because cleanup is not registered until #execute reaches its finally, so this close resolves and the probe later creates a detached teardown and plugin-data cleanup. Include the in-flight probe promises in the settlement fence so shutdown cannot finish while their transports and temporary directories remain active.

Useful? React with 👍 / 👎.

Fixed in this PR (859f375). McpProbeService.settle() now fences the service's in-flight probe promises (#inFlight) as well as #pendingTeardowns, looping until both are empty — so server.close() cannot resolve while a probe is still connecting/listing tools; once that probe reaches its response boundary it registers its teardown, which the same loop then awaits (bounded by the 10 s teardown cap). Proof: new unit test mcp-probe-service.test.ts :: settle() fences in-flight probes, not only already-registered teardowns (#397 review) blocks a probe inside connect(), calls settle(), asserts it is still pending 50 ms later, releases the connect, and asserts settle resolves only after the report returned, the (slow) transport closed, and the plugin-data directory is gone. Red on the previous commit, green now.

Document the package bin change needed to enable installers

For a portable-only scaffold, rewriteManifest removes the installer entry from package.json; adding claude, codex, or cursor only to agent-bundle.config.ts later does not restore that npm bin mapping. The build may emit the installer file, but the documented npx command remains unavailable after publishing, so these generated instructions should also tell users to restore the corresponding package.json entry (or preserve a mechanism that regenerates it).

Useful? React with 👍 / 👎.

Fixed in this PR (c53c000). The portable-only README rewrite in rewriteReadmeInstall now states that the installer's bin entry was dropped from package.json and names the exact mapping to restore — "<installer-bin>": "./dist/bin/<installer-bin>.js" — in both the shell-comment example and the prose, alongside the config-target change, and notes that the build emits the installer file but never edits the manifest. Test: scaffold.test.ts :: renders README install instructions for the selected targets now derives the dropped entry from the template's shipped package_json bin map (after plugin-name substitution) and asserts the portable-only README contains it in both places, so template drift in the bin path fails the test.

Protect cleanup from synchronous close failures

If either injected or SDK transport's close() throws synchronously, evaluation of the array passed to Promise.allSettled aborts before this cleanup is registered (and a throwing client.close() also skips transport.close()). Because #run no longer has its former outer finally, that scenario turns the probe into a rejection and permanently leaks its plugin-data directory; wrap each close invocation so synchronous throws become settled promises and always reach #removePluginDataAfter.

Useful? React with 👍 / 👎.

Fixed in this PR (c53c000). Added a module-level settledClose(close) that invokes the callback inside try/catch and normalizes both a synchronous throw and a rejection into one settled promise. #execute's finally now builds the teardown as Promise.allSettled([settledClose(() => client.close()), settledClose(() => transport.close())]), so a throwing client.close() neither skips transport.close() nor aborts before #removePluginDataAfter is registered; the #withinBudget timeout paths use the same wrapper for onTimeout. Proof (both red before, green now): mcp-probe-service.test.ts :: still closes the transport and removes plugin data when a close() throws synchronously (#397 review) (client close throws synchronously → report is still ok, transport closed, plugin data removed after settle()), and :: reports a timeout even when the timeout teardown throws synchronously (spent-budget connect path with a synchronously throwing transport close → honest timed-out/connect report, plugin data removed).

Redact URL userinfo before exempting links

When a probed server returns instructions, titles, or tool descriptions containing an HTTP URL with Basic-auth userinfo (for example, https://alice:hunter2@example.test/private), redactCredentialText does not recognize the password and this new :// exemption makes hasAbsolutePath return false, so the complete credential is emitted in the browser-facing probe report. Preserve ordinary documentation URLs, but strip or reject URL userinfo before allowing the URL through.

Useful? React with 👍 / 👎.

Fixed in this PR (0761d34). redactProbeText now masks URL userinfo before the absolute-path check: urlUserinfoPattern = /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#@"'<>]+@/giu rewrites scheme://user[:secret]@host… to scheme://[REDACTED]@host… for every scheme (not just http/https), while a plain email address (no ://) and ordinary documentation URLs pass through unchanged. Test: extended mcp-probe-service.test.ts :: keeps URLs while redacting real absolute and bundle paths (#316 review) with https://alice:hunter2@example.test/private, postgres://svc:pa55@db.internal:5432/app, a bare-user https://alice@example.test/private, and an email ops@example.test; asserts the masked forms and that the serialized report contains none of hunter2, pa55, alice. Red on the previous commit (the credential appeared verbatim), green now.

Retry cleanup after a capped teardown settles

When a transport remains alive past the 10-second cap and still holds files under pluginData—notably on Windows—the cap wins this race and rm can fail with EPERM; the following rejection handler converts that failure to success, and nothing retries removal if the transport later finishes closing. Consequently settle() and Workbench shutdown can report completion while leaving the probe directory behind. Preserve a retry chained to the eventual teardown settlement rather than permanently swallowing the capped removal failure.

Useful? React with 👍 / 👎.

Fixed in this PR (e85271d). #removePluginDataAfter now records whether the cap won the race; if it did and the removal then fails, one more rm is chained to the teardown's eventual settlement and tracked in #pendingTeardowns via a new #track helper, so settle() (and therefore Workbench shutdown) does not report completion while the directory is still held — the failure is no longer swallowed for good. Removal itself uses rm({ force, recursive, maxRetries: 3, retryDelay: 50 }) so transient EPERM/EBUSY are retried in place. A pluginDataTeardownCapMs option was added as a testing seam (production keeps the 10 s constant). Test: mcp-probe-service.test.ts :: retries plugin-data removal once a capped teardown finally settles (#397 review) — plugin data under a read-only parent stands in for the held directory, cap = 100 ms, transport close stalls; asserts the first removal fails (proof file still present after 250 ms), settle() stays pending, and once the parent is writable and the close is released the fence resolves only after the chained retry removed the directory. Red on the previous commit, green now.

Bound the cleanup retry tracked by shutdown

When the teardown cap fires, removePluginData fails (for example with EPERM on Windows), and the faulty transport never settles, this newly added retry remains in #pendingTeardowns forever because it waits directly on teardown. The fresh evidence is that closeForeground() now awaits mcpProbe.settle(), whose loop cannot finish while this retry remains tracked, so the cap no longer bounds Workbench shutdown in the exact stalled-close scenario it is intended to guard; bound or otherwise release the retry while preserving best-effort eventual cleanup.

Useful? React with 👍 / 👎.

Fixed in this PR (90772c0). The retry chained to the teardown's eventual settlement is now detached from the settle() fence (it is no longer added to #pendingTeardowns): settle() — and therefore closeForeground() — stays bounded by the cap even when a faulty transport never settles, while the retry still removes the directory best-effort if the transport does eventually close. The doc comment on #removePluginDataAfter states this trade-off explicitly. Test updated: mcp-probe-service.test.ts :: retries plugin-data removal once a capped teardown finally settles (#397 review) now asserts that settle() resolves within 500 ms while the transport is still stalled and the held directory is still present (a fence that waited on the transport fails the test), and that after the transport is released the chained retry removes the directory (event order settled → transport-closed → plugin-data-removed).

Avoid relying on mode bits to force removal failure

When the unit suite runs as root in a container, or on a platform such as Windows where POSIX directory mode bits are not enforced equivalently, chmod(parent, 0o555) does not guarantee that rm(pluginData, { recursive: true }) will fail. The capped cleanup can therefore delete proof.txt, making the assertion at line 651 fail even though the implementation behaves correctly; simulate the removal rejection through an injected seam or otherwise use a platform-independent failure mechanism.

Useful? React with 👍 / 👎.

Fixed in this PR (b3af919). Added a removePluginData testing seam to McpProbeServiceOptions (production default is the recursive rm) and rewrote the test to inject the failure: while the fake transport is alive the injected removal throws an EPERM error, and after the transport closes it delegates to the real rm. No mode bits are involved, so the failure is identical as root, in containers, and on Windows. The test now also asserts the event order removal-rejected → settled → transport-closed → plugin-data-removed.

Make the owner-marker assertion path-portable

When this test runs on Windows, os.tmpdir() records a drive-letter or UNC path such as C:\Temp in temporaryRoot, while this assertion unconditionally requires the value to begin with /. The production helper explicitly supports win32, so the unit suite will fail on that supported path shape; assert with path.isAbsolute() or use a platform-aware matcher.

Useful? React with 👍 / 👎.

Fixed in this PR (b3af919). The assertion now uses path.isAbsolute(owner.temporaryRoot) instead of /^\//, so drive-letter (C:\Temp) and UNC roots on Windows satisfy it alongside POSIX /tmp; the rest of the marker (cwd, pid, workerId, and temporaryRoot !== root) is asserted as before.

Track the first timeout close before cleaning plugin data

When the budget is already exhausted and the first transport.close() remains pending, this fire-and-forget promise is not included in #execute's teardown fence. #execute immediately invokes close() again; for a non-reentrant transport whose duplicate close returns early, that second promise lets plugin-data removal run while the original TERM/KILL sequence is still active. On Windows the removal can then fail with capWon === false, so it is neither retried nor reported. Reuse or otherwise pass the first close promise into the bounded teardown so cleanup follows the actual close without delaying the probe response.

Useful? React with 👍 / 👎.

Fixed in this PR (acc03ce). #execute now owns a single memoized closeTransport() (transportClose ??= settledClose(() => transport.close())); both #withinBudget timeout hooks (connect and protocol) and the teardown in finally call it, so the teardown fence — and the plugin-data removal chained behind it — awaits the very close a timeout already started rather than a duplicate call a non-reentrant transport answers immediately. Test: mcp-probe-service.test.ts :: chains plugin-data removal to the close a timeout already started, not a duplicate close (#397 review) — spent-budget connect path, transport whose first close stalls and whose duplicate close returns at once; asserts close() was invoked exactly once, the plugin data survives while the first close is pending, and it is removed after that close is released and settle() resolves. Red on the previous commit (the duplicate close let removal run early), green now.

Redact userinfo through the final @ delimiter

When a server returns a URL whose password contains a raw @, such as https://alice:pa@ss@example.test/private, WHATWG URL parsing treats the final @ as the authority delimiter and the earlier one as part of the password. This pattern stops at the first @, producing https://[REDACTED]@ss@example.test/private and exposing part of the credential in the browser-facing report; excluded characters such as quotes can prevent matching altogether despite also being accepted and encoded by URL parsers. Fresh evidence in the replacement regex is its explicit exclusion of @ and other valid userinfo characters, so redact through the final authority delimiter or parse URLs before masking.

Useful? React with 👍 / 👎.

Fixed in this PR (4ee7483). urlUserinfoPattern is now /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#\\]*@/giu: the authority run excludes only whitespace and the WHATWG authority terminators (/, ?, #, \) and is greedy, so the match extends through the final @ before the path — the delimiter URL parsers honour — and no longer excludes @, quotes, or other characters parsers accept in userinfo. An @ in the query/fragment is untouched. Test: added https://alice:pa@ss@example.test/private?next=me@x and https://al"ice:s3cret@example.test/#top to :: keeps URLs while redacting real absolute and bundle paths (#316 review); asserts both become https://[REDACTED]@example.test/... with ?next=me@x preserved, and that the serialized report contains none of pa@ss, @ss@, s3cret. With the previous pattern the test reproduces exactly the leak you described (https://[REDACTED]@ss@example.test/... and the quoted credential verbatim); green now.

Redact backslashes in non-special URL userinfo

When probe text contains userinfo in a non-special URL such as postgres://alice:se\cret@example.test/db, WHATWG parsing accepts the backslash as part of the password, but this pattern stops at \ before reaching @ and performs no masking; hasAbsolutePath also does not reject a single backslash, so the full credential reaches the browser-facing report. Fresh evidence beyond the prior userinfo fix is the final regex's explicit \ exclusion, which is valid as an authority terminator for HTTP(S) but not for schemes such as the already-supported postgres:// example.

Useful? React with 👍 / 👎.

Fixed in this PR (281520f). \ is no longer an authority terminator in urlUserinfoPattern — now /\b([a-z][a-z0-9+.-]*:\/\/)[^\s/?#]*@/giu, stopping only at whitespace or the terminators every WHATWG scheme shares (/, ?, #). For non-special schemes the backslash is userinfo and is masked; for special schemes this errs toward redaction, which is the safe direction for a browser-facing report. The doc comment records that trade-off. Test: added postgres://alice:se\cret@example.test/db and https://bob:pw\x@example.test/ to :: keeps URLs while redacting real absolute and bundle paths (#316 review); asserts both become scheme://[REDACTED]@example.test/... and that the serialized report contains neither cret nor bob. With the previous pattern the test fails with the credential verbatim; green now.

Redact paths hidden behind local URI schemes

When probe text contains a local-resource URI such as unix:///home/alice/private.sock or vscode://file/home/alice/project, every slash in the absolute path follows either : from :// or another slash, so this expression returns false and the full machine-local path reaches the browser-facing probe report. Preserve documentation URLs by exempting only appropriate network schemes such as HTTP(S), or explicitly classify local-resource URI paths before applying the exemption.

Useful? React with 👍 / 👎.

Fixed in this PR (242add5). The URL exemption is now limited to network schemes: a new localUriPathPattern = /\b(?!(?:https?|wss?):\/\/)[a-z][a-z0-9+.-]*:\/\/[^\s/]*\//iu makes hasAbsolutePath return true for any other scheme://<authority>/… — so unix:///home/alice/private.sock, vscode://file/home/alice/project, postgres://host/db, and file: all fail closed like a bare absolute path, while oci://registry.example.test (no path component) and mailto: are untouched. Userinfo masking still runs first for every scheme. Test: :: keeps URLs while redacting real absolute and bundle paths (#316 review) now has fixtures for the empty-authority form, the authority-then-path form, a non-network scheme with a path, and a non-network scheme without one; asserts the first three are [REDACTED] and the last is preserved, and that alice never appears in the serialized report. Red on the previous commit (both local URIs reached the report verbatim), green now.

Retry removal after an early close failure

When close() throws or rejects while a Windows child still briefly holds pluginData, settledClose makes the teardown appear complete, the immediate rm can fail with EPERM, and this branch discards that failure because the cap did not win; settle() then completes with the directory permanently left behind. Fresh evidence in the current code is the explicit if (!capWon) return, so schedule a bounded delayed retry for this early-failure path as well.

Useful? React with 👍 / 👎.

Fixed in this PR (242add5). In the !capWon branch (teardown settled — possibly because a close failed fast — but the removal still failed), #removePluginDataAfter now schedules one bounded retry after mcpProbePluginDataRetryDelayMs (250 ms) and tracks it via #track in #pendingTeardowns, so settle() / Workbench shutdown waits for that retry and the fence stays bounded. The capped-and-stalled branch keeps its best-effort, unfenced retry as discussed on the earlier thread. Test: mcp-probe-service.test.ts :: retries removal once, fenced, when the teardown settled but the directory was still held (#397 review) — transport close rejects immediately, the injected removal throws EPERM on the first call and delegates to rm on the second; asserts settle() resolves only after two removals and the directory is gone. Red on the previous commit (one removal, directory left behind), green now.

Redact whitespace-bearing URL userinfo

When a probed server returns a valid URL whose password contains raw whitespace, such as https://alice:se cret@example.test/private, WHATWG parsing percent-encodes the space (and strips embedded tabs/newlines), but this pattern stops at \s before reaching @ and performs no masking. The absolute-path check also exempts the URL, so the complete credential is emitted in the browser-facing probe report. Fresh evidence beyond the earlier delimiter fixes is the final pattern's explicit \s exclusion; parse the URL or allow whitespace through the authority match before masking.

Useful? React with 👍 / 👎.

Fixed in this PR (399122c). urlUserinfoPattern is now /\b([a-z][a-z0-9+.-]*:\/\/)[^/?#]*@/giu: the authority run stops only at the three terminators every WHATWG scheme shares (/, ?, #) and is greedy through the final @, so whitespace (encoded or stripped by parsers) — like @, quotes and \ before it — can no longer end the mask short of the credential. The doc comment now states the deliberate consequence: a path-less URL followed by an @ before any /, ?, # is over-masked as if it were userinfo, which is the safe side for a browser-facing report. Test: :: keeps URLs while redacting real absolute and bundle paths (#316 review) gains https://alice:se cret@example.test/private and a tab-bearing https://bob:x\ty@example.test/ (both → https://[REDACTED]@example.test/…), plus a fixture pinning the documented over-masking trade-off. Red on the previous pattern (the space-bearing credential appeared verbatim), green now.